perf(sdk): memoize history selectors and amount conversion factors (#32) - #413
perf(sdk): memoize history selectors and amount conversion factors (#32)#413RaymondAbiola wants to merge 1 commit into
Conversation
|
@RaymondAbiola Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Jaydbrown
left a comment
There was a problem hiding this comment.
Not mergeable as-is — two real problems, both blocking:
-
Removes strictness flags from all three
tsconfigfiles repo-wide (tsconfig.json,tsconfig.rollup.json,packages/react/tsconfig.json):- "noImplicitAny": true, - "useUnknownInCatchVariables": true,
This is a real regression in the codebase's type-safety guarantees, applying to every file in the SDK and the React package, not just the selector-memoization work this PR is about. This almost certainly exists to let the new code type-check without properly typing something (implicit
anys or untypedcatchbindings) rather than being an intentional, discussed policy change. Please restore both flags and fix whatever type errors that surfaces in the new code instead. -
This branch is stale and would silently revert an already-merged fix.
src/dashboard/transaction-history.ts'stoIso8601/SPACE_SEPARATED_DATETIMEhelper (added by PR #406, "normalize space-separated indexer timestamps to ISO 8601 for Safari") is removed by this diff, along with its regression test insrc/tests/transaction-history-crash.test.ts. GitHub shows this as a clean, conflict-free diff since the textual regions don't overlap — but merging as-is would reintroduce the Safari timestamp bug #406 just fixed. Please rebase onto currentmainand drop this reversion; the actual memoization work intransaction-history.ts's selectors doesn't need to touchasTimestamp/toIso8601at all.
The core selector-memoization approach and the utils.ts amount-conversion caching look reasonable on a skim, but I'd like to see this rebased and with the tsconfig changes reverted before a full re-review.
|
Ok thanks for your review. I will fix it. |
…lter predicate (#32)
7af0584 to
14140bf
Compare
|
@Jaydbrown Thanks for the review — rebased onto current On the tsconfig flags: the branch never actually touched any tsconfig file. On the rest: the rebase showed the optimizations here had already landed:
I resolved every conflict in favor of I also verified the |
Closes #366